Skip to content

Add taper support and refactor array-factor calculation #2726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

George-Guryev-flxcmp
Copy link
Contributor

@George-Guryev-flxcmp George-Guryev-flxcmp commented Aug 8, 2025

Add taper support to RectangularAntennaArrayCalculator; refactor array factor computation for clarity and efficiency.

  • Implemented taper integration
  • Refactored AF calculation
  • Updated tests and docs

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, 7 comments

Edit Code Review Bot Settings | Greptile

Copy link
Contributor

github-actions bot commented Aug 8, 2025

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

  • tidy3d/plugins/microwave/array_factor.py (86.5%): Missing lines 654,703,801-802,805-807,809,812-813,912,918,940,960,980,1000,1053

Summary

  • Total: 126 lines
  • Missing: 17 lines
  • Coverage: 86%

tidy3d/plugins/microwave/array_factor.py

Lines 650-658

  650             amp_z = amp_z[:, None, None]
  651 
  652         # Tapers with non-separable amplitude weights are not supported by this function
  653         else:
! 654             raise ValueError(f"Unsupported taper type {type(self.taper)} was passed.")
  655 
  656         # Calculate individual array factors in x, y, and z directions
  657         af_x = np.sum(
  658             amp_x * exp_x,

Lines 699-707

  699         amps = self.taper.amp_multipliers(self.array_size)
  700 
  701         # ensure amplitude weights are in format tuple[ArrayLike, ]
  702         if len(amps) != 1:
! 703             raise ValueError(
  704                 "Non-cartesian taper was expected. Please ensure a valid taper is used."
  705             )
  706 
  707         # compute array factor: AF(theta,f) = sum_{x,y,z} amp(x,y,z) * exp_x(x,theta,f)*exp_y(y,theta,f)*exp_z(z,theta,f)

Lines 797-817

  797     def _antenna_amps(self) -> ArrayLike:
  798         """Amplitude multipliers of antennas in an array."""
  799 
  800         if self.taper is not None:
! 801             if isinstance(self.taper, RectangularTaper):
! 802                 amp_x, amp_y, amp_z = self.taper.amp_multipliers(self.array_size)
  803 
  804                 # broadcast amplitudes to [N_x,1,1], [N_y,1,1] and [Nz,1,1], respectively
! 805                 amp_x = amp_x[:, None, None]
! 806                 amp_y = amp_y[None, :, None]
! 807                 amp_z = amp_z[None, None, :]
  808 
! 809                 amps = amp_x * amp_y * amp_z
  810 
  811             else:
! 812                 amps = self.taper.amp_multipliers(self.array_size)
! 813             return np.ravel(amps)
  814 
  815         amps_per_dim = [
  816             np.ones(size) if multiplier is None else multiplier
  817             for multiplier, size in zip(self.amp_multipliers, self.array_size)

Lines 908-916

  908     """This class provides interface for window selection."""
  909 
  910     def _get_weights_discrete(self, N: int) -> ArrayLike:
  911         """Interface function for computing window weights at N points."""
! 912         raise Tidy3dNotImplementedError(
  913             f"Calculation of antenna amplitudes at a discrete number of points is not yet implemented for window type {self.type}."
  914         )
  915 
  916     def _get_weights_continuous(self, p_vec: ArrayLike) -> ArrayLike:

Lines 914-922

  914         )
  915 
  916     def _get_weights_continuous(self, p_vec: ArrayLike) -> ArrayLike:
  917         """Interface function for computing window weights at given locations."""
! 918         raise Tidy3dNotImplementedError(
  919             f"Calculation of antenna amplitudes at arbitrary locations is not yet implemented for window type {self.type}."
  920         )
  921 

Lines 936-944

  936         -------
  937         ArrayLike
  938             1D array of Hamming window weights.
  939         """
! 940         return hamming(N)
  941 
  942 
  943 class BlackmanWindow(AbstractWindow):
  944     """Standard Blackman window for tapering or spectral shaping."""

Lines 956-964

  956         -------
  957         ArrayLike
  958             1D array of Blackman window weights.
  959         """
! 960         return blackman(N)
  961 
  962 
  963 class BlackmanHarrisWindow(AbstractWindow):
  964     """Standard Blackman-Harris window for tapering or spectral shaping."""

Lines 976-984

  976         -------
  977         ArrayLike
  978             1D array of Blackman-Harris window weights.
  979         """
! 980         return blackmanharris(N)
  981 
  982 
  983 class HannWindow(AbstractWindow):
  984     """Hann window with configurable sidelobe suppression and sidelobe count."""

Lines 996-1004

   996         -------
   997         ArrayLike
   998             1D array of Hann window weights.
   999         """
! 1000         return hann(N)
  1001 
  1002 
  1003 class ChebWindow(AbstractWindow):
  1004     """Standard Chebyshev window for tapering with configurable sidelobe attenuation."""

Lines 1049-1057

  1049         -------
  1050         ArrayLike
  1051             1D array of Kaiser window weights.
  1052         """
! 1053         return kaiser(N, self.beta)
  1054 
  1055 
  1056 class TaylorWindow(AbstractWindow):
  1057     """Taylor window with configurable sidelobe suppression and sidelobe count."""

Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start! Still not sure about at, sll, nbar, etc vs attenuation, side_lobe_level, num_constant_sidelobes, etc. @yaugenst what's our policy on using pydantic alias feature on frontend?

Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work!

Comment on lines 1160 to 1164
mode: Literal["1d", "radial"] = pd.Field(
default="1d",
title="Mode of Taylor Window",
description="Desired mode of Taylor window.",
)
Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is there a way we can avoid this mode switch? What if we just define

class AbstractWindow(Tidy3dBaseModel, ABC):
    """This class provides interface for window selection."""

    def _get_weights_discrete(self, N: int) -> ArrayLike:
        """Interface function for computing window weights at N points."""
        raise Tidy3DNotImplemented(f"Calculation of antenna amplitudes at a discrete number of points is not yet implemented for window type {self.type}.")

    def _get_weights_continuous(self, p_vec: ArrayLike) -> ArrayLike:
        """Interface function for computing window weights at given locations."""
        raise Tidy3DNotImplemented(f"Calculation of antenna amplitudes at arbitrary locations is not yet implemented for window type {self.type}.")

then RectangularTaper can call _get_weights_discrete() while RadialTaper can call _get_weights_continuous(). TaylorWindow would have valid implementation for both, while all the others would only have _get_weights_discrete() for now. Would that work here? I think that would simplify the code

… calculation

- Implemented taper integration to allow amplitude weighting in antenna arrays
- Refactored array factor computation for improved clarity and efficiency
- Updated relevant tests and documentation accordingly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants